All committers have signed the CLA.

CI failure looks related to some issues on master (should go away with a rebase).

While you're rebasing, please include the entire PR description in your commit message.

@tamird Rebased with master and included the description in the commit message

Thanks for your help. Some minor comments left.

Reviewed 3 of 6 files at r1.
Review status: 3 of 6 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/acceptance/java/TestRunner.java, line 31 at r1 (raw file):

    public static void main(String[] args) throws Exception {
        if (args.length < 2) {
            throw new Exception("unexpected : missing arguments. usage : java TestRunner [SUCCESS/FAIL] [dir]");

In English there is no space between a colon (:) and the previous word. e.g. "unexpected: missing ..." (also see below)
Put "usage: ..." on a different line; e.g. "unexpected: missing arguments\nusage: java ..."

pkg/acceptance/java/TestRunner.java, line 37 at r1 (raw file):

        ExpectedOutcome expectedOutcome = ExpectedOutcome.parse(mode);
        if (expectedOutcome == null) {
            throw new Exception("unexpected : missing arguments. usage : java TestRunner [SUCCESS/FAIL] [dir]");

The null value doesn't say the argument was missing, but rather that it wasn't correct. Make the exception more informative.

pkg/acceptance/java/TestRunner.java, line 88 at r1 (raw file):

        for (File file : files) {
            if (!file.getName().contains(".java")) {

Simplify: if (file.getName().contains(".hava")) { javaFiles.add(file); }

also, use endsWith not contains.

pkg/acceptance/java/TestRunner.java, line 179 at r1 (raw file):

        }
    }
}

Missing newline at end of file.

pkg/acceptance/java/fail/JavaFailureTest.java, line 46 at r1 (raw file):

            throw new Exception("unexpected: DROP TABLE reports " + res + " rows changed, expecting 0");
        }
        stmt = conn.prepareStatement("SELECT 1, 2 > ?, ?::int, ?::string, ?::string, ?::string, ?::string, ?::string");

Move this test into a separate file.

pkg/acceptance/java/fail/JavaFailureTest.java, line 67 at r1 (raw file):

            throw new Exception("unexpected");
        }
        stmt = conn.prepareStatement("CREATE TABLE accounts (id INT PRIMARY KEY, balance INT, cdate DATE)");

Move this test (this CREATE together with the INSERT below) into a separate file.

pkg/acceptance/java/success/JavaSuccessTest.java, line 48 at r1 (raw file):

            throw new Exception("unexpected: DROP TABLE reports " + res + " rows changed, expecting 0");
        }
        stmt = conn.prepareStatement("SELECT 1, 2 > ?, ?::int, ?::string, ?::string, ?::string, ?::string, ?::string");

Split test here (see comment above)

pkg/acceptance/java/success/JavaSuccessTest.java, line 69 at r1 (raw file):

            throw new Exception("unexpected");
        }
        stmt = conn.prepareStatement("CREATE TABLE accounts (id INT PRIMARY KEY, balance INT, cdate DATE)");

Split test here (see comment above)

Comments from Reviewable

Reviewed 4 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, 14 unresolved discussions, all commit checks successful.

pkg/acceptance/java/run_cleanup.sh, line 4 at r1 (raw file):

# cleans all {.class, .pk8} files in the current directory
find . -type f -name "*.class" -exec rm -f {} \;

find . -type f -name '*.class' -or -name '*.pk8' -exec rm -f {} \;

pkg/acceptance/java/run_java_test.sh, line 2 at r1 (raw file):

#!/usr/bin/env bash
set -e

set -euo pipefail

pkg/acceptance/java/run_java_test.sh, line 9 at r1 (raw file):

export CLASSPATH=$CLASSPATH:$JAVA_ROOT/success:$JAVA_ROOT/fail:$JAVA_ROOT

cd $JAVA_ROOT

how come this is necessary?

pkg/acceptance/java/TestRunner.java, line 9 at r1 (raw file):

/*
This class compiles and runs all java files under a directory.

perhaps i'm missing something, but why is this test runner written in java? it seems to me that this thing amounts to little more than shelling out to various tools, and that there is little or no benefit to it being written in java.

given that java is not a core competency for us, I would prefer to see this written in Go.

pkg/acceptance/java/TestRunner.java, line 59 at r1 (raw file):

                        break;
                    }
                    // This test should have "PASSED" but "FAILED

broken sentence.

pkg/acceptance/java/fail/JavaFailureTest.java, line 7 at r1 (raw file):

    public static void main(String[] args) throws Exception {
        Class.forName("org.postgresql.Driver");
        String DB_URL = "jdbc:postgresql://";

this connection string construction code is duplicated; can it be shared?

Comments from Reviewable

Review status: 4 of 6 files reviewed at latest revision, 14 unresolved discussions, all commit checks successful.

pkg/acceptance/java/TestRunner.java, line 9 at r1 (raw file):

Not specific reason. It was as per the suggestion "the different Java tests should be executed one after another by test logic written in Java" mention in the issue(#4578) description. I agree that this should be written in go. I will rewrite this in go.

Comments from Reviewable

@knz, @dt, @mjibson what remains to be done here?

At the very least we need to rebase it, in particular to pick up 66c28b5.

Beyond that, looks like there's @tamird's objection to the loop that runs the individual tests being on the java side rather than in Go, but that potentially could get changed later if/when it becomes a pain point.

@dt could we get this merged, assuming it's straightforward? And update / close #4591.

I'm going to close this since it appears to have died and #4591 was closed by #19321.